retry connection prelude on connect errors #276
Conversation
nerdrew
force-pushed
the
wrongpass-retry
branch
from
ab5415c to
aba5b7b
Compare
| rescue FailoverError, CannotConnectError => error | ||
| error._set_config(config) | ||
| raise error | ||
| rescue ConnectionError => error | ||
| connect_error = CannotConnectError.with_config(error.message, config) | ||
| connect_error.set_backtrace(error.backtrace) | ||
| raise connect_error | ||
| rescue CommandError => error | ||
| if error.message.match?(/ERR unknown command ['`]HELLO['`]/) | ||
| raise UnsupportedServer, | ||
| "redis-client requires Redis 6+ with HELLO command available (#{config.server_url})" | ||
| else | ||
| raise | ||
| end |
There was a problem hiding this comment.
I don't understand why you moved these rescues. Now only the send_prelude call in connect is covered.
There was a problem hiding this comment.
I closed this in favor of the other PR, but for posterity: I didn't move the rescues. The git diff just looks wonky. The actual diff just extracted the "send prelude" logic into a method that could be used in 2 different places.
| assert_match(/WRONGPASS invalid username-password pair/, error.message) | ||
|
|
||
| # Correct password, but user disabled | ||
| backup.call("ACL", "SETUSER", "AzureDiamond", "<hunter2", ">trolilol", "off") |
There was a problem hiding this comment.
Mutating an existing user, and trying to fix it in a rescue isn't good, as it may fail and leave the server in a state that fail other tests.
You can create a different user.
There was a problem hiding this comment.
I can create a different user for this one. But for the tests around a disabled default user need to mutate the existing user. Do you have a preference how to do that? I can try to restore the default user after the test OR I can kill and restart the redis server after...
There was a problem hiding this comment.
(These tests are the same between the 2 PRs, so even though this PR is closed, this is still relevant to the other PR.)
There was a problem hiding this comment.
Less mutation is always better. So if you can't absolutely eliminate them all it's OK, but we should still thrive to have as little as possible.
There is also the option of starting one more server with a disabled default user. Redis servers are quite cheap.
|
Closing in favor of #275 |
2 participants
No description provided.